Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syntax-highlight regex prompts #7738

Merged
merged 4 commits into from
Jul 27, 2023
Merged

Syntax-highlight regex prompts #7738

merged 4 commits into from
Jul 27, 2023

Conversation

the-mikedavis
Copy link
Member

We can re-use the code-block highlighting function from the markdown component to highlight regex prompts with tree-sitter-regex. To make this change a little cheaper, we switch some functions from taking &Ropes to RopeSlices (see 50557c6's message).

We can use tree-sitter-regex highlighting in prompts for entering
regexes, like `search` or `global_search`. The `highlighted_code_block`
function from the markdown component makes this a very small change.

This could be improved in the future by leaving the parsed syntax tree
on the prompt, allowing incremental updates. Prompt lines are usually so
short though and tree-sitter-regex is rather small and uncomplicated,
so that improvement probably wouldn't make a big difference.
@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-tree-sitter Area: Tree-sitter A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jul 25, 2023
@@ -50,4 +50,3 @@
])

(class_character) @constant.character
(pattern_character) @string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the (ERROR) class so that invalid regexes are highlighted? That's something that has been a TODO in the prompt for a while, the idea was to highlight the whole line as red but I guess we can actually do better and use (ERROR) nodes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This TODO:

// TODO: mark command line as error
kakoune just colors the whole line as red)

Copy link
Member

@pascalkuthe pascalkuthe Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error highlights don't quite match the rust regex syntax. Unclosed brackets don't seem to be highlighted for example. That kind of makes sense since the regex grammar is not just specific to the rust regex engine so it might be more permissive (since usually rejecting invalid syntax is not a goal for a syntax highlighters).

So I think it would still make sense to implement #3050 (comment) as a separate PR in the future (although the error highlighting is nice)

Copy link
Member Author

@the-mikedavis the-mikedavis Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could overlay some error highlights for regex syntax errors according to rust's regex without too much extra work. We could check the syntax with the regex-syntax dep and merge the location span's range in the additional_highlight_spans:

pub fn highlighted_code_block<'a>(
text: String,
language: &str,
theme: Option<&Theme>,
config_loader: Arc<syntax::Loader>,
additional_highlight_spans: Option<Vec<(usize, std::ops::Range<usize>)>>,
) -> Text<'a> {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's a really neat idea 👍 Although you need to be sure to convert byte/utf8 indeces to char/utf32 indices

@the-mikedavis the-mikedavis force-pushed the prompt-regex-highlighting branch from fb103ee to c23edfb Compare July 26, 2023 15:36
the-mikedavis and others added 3 commits July 26, 2023 10:36
Since regex is almost always injected into other languages,
`pattern_character`s will inherit the highlight for the structure that
injects them (for example `/foo/` in JavaScript or `~r/foo/` in Elixir).
This removes the string highlight when used in the prompt.

We also add `ERROR` node highlighting so that errors in regex syntax
appear in the prompt. This resolves a TODO in the `regex_prompt`
function about highlighting errors in the regex.
Pascal and I discussed this and we think it's generally better to
take a 'RopeSlice' rather than a '&Rope'. The code block rendering
function in the markdown component module is a good example for how
this can be useful: we can remove an allocation of a rope and instead
directly turn a '&str' into a 'RopeSlice' which is very cheap.

A change to prefer 'RopeSlice' to '&Rope' whenever the rope isn't
modified would be nice, but it would be a very large diff (around 500+
500-). Starting off with just the syntax functions seems like a nice
middle-ground, and we can remove a Rope allocation because of it.

Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
This removes a handful of allocations for functions calling into the
function, which is nice because the prompt may call this function on
every keypress.
@the-mikedavis the-mikedavis force-pushed the prompt-regex-highlighting branch from c23edfb to ef47075 Compare July 26, 2023 15:37
@archseer archseer merged commit 953073a into master Jul 27, 2023
@archseer archseer deleted the prompt-regex-highlighting branch July 27, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-tree-sitter Area: Tree-sitter C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants